Skip to content

Run unit tests with codegen diff check#8262

Closed
bryantbiggs wants to merge 2 commits intomainfrom
fix/check-codegen
Closed

Run unit tests with codegen diff check#8262
bryantbiggs wants to merge 2 commits intomainfrom
fix/check-codegen

Conversation

@bryantbiggs
Copy link
Contributor

@bryantbiggs bryantbiggs commented Feb 28, 2025

Description

  • Update codegen to use new script/format since we've updated to a version that has since changed the script/format
  • Run unit tests with codegen diff check on every PR to catch this in the future and ensure we update the generated files

This would have caught issues with #8242 and #8210

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@bryantbiggs bryantbiggs added skip-release-notes Causes PR not to show in release notes area/ci labels Feb 28, 2025
@bryantbiggs
Copy link
Contributor Author

this is expected to fail on the first go - making sure it catches this failure

@bryantbiggs bryantbiggs marked this pull request as draft February 28, 2025 01:10
@bryantbiggs bryantbiggs marked this pull request as ready for review February 28, 2025 02:14
@dims dims enabled auto-merge (squash) February 28, 2025 16:43
@cheeseandcereal
Copy link
Member

cheeseandcereal commented Feb 28, 2025

When we run code generation, new versions of the aws golang v2 sdk are automatically fetched, and used in codegen for go generate ./pkg/awsapi/...

Won't this make it so that, if any of the aws services we use in that package are updated, this will just start failing the test-and-build check until someone updates and merges a fix, and everyone else rebases? Either that, or every PR is going to have to include autogen updates from AWS service updates in each of their PRs, and keep them up to date as AWS keeps their service up to date, making stale PRs more difficult to deal with?

I'm not convinced this is the right approach here.

Maybe we should either change the make build target to not run the auto-gen, or isolate the part about dynamically fetching new versions of the aws sdk for auto-gen to its own gen target that's not run by default or something? Then I would be more OK with checking for auto-gen diff like this if it doesn't rely on dynamically updating external dependencies.

@dims dims removed their request for review February 28, 2025 18:15
auto-merge was automatically disabled February 28, 2025 22:22

Pull request was closed

@bryantbiggs bryantbiggs deleted the fix/check-codegen branch February 28, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci skip-release-notes Causes PR not to show in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants